Skip to content

Homescreen and Infoscreen#11

Merged
LinkKF merged 45 commits intodevelopmentfrom
new-components
Mar 31, 2021
Merged

Homescreen and Infoscreen#11
LinkKF merged 45 commits intodevelopmentfrom
new-components

Conversation

@LinkKF
Copy link
Copy Markdown
Collaborator

@LinkKF LinkKF commented Mar 27, 2021

This PR adds the Homescreen and the Infoscreen for the website. The mdx "code" to test it lies in the content file "contentPage". The filter from the Homescreen is removed, but I work on getting it implemented again. On the info pages, the additional Infos are, other than we said earlier, aside the picture instead of under it. I think like that its more clear, where it belongs to.

@LinkKF LinkKF requested a review from Quaffel March 27, 2021 14:19
Copy link
Copy Markdown
Owner

@Quaffel Quaffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good overall, but I have some requests. Please address them and re-request my review once you're done. And don't forget to resolve the merge conflicts. If you have any questions, please don't mind asking!

Comment thread frontend/src/components/mdx/textField/textField.tsx Outdated
Comment thread frontend/src/components/mdx/info/info.tsx Outdated
Comment thread frontend/src/components/mdx/info/info.tsx
Comment thread frontend/content/contentPage.mdx Outdated
Comment thread frontend/src/components/mdx/image/image.tsx
Comment thread frontend/src/components/mdx/image/image.tsx
left: 0;
bottom: 0;
right:0;
width:100vw;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these properties as they'd hide the navbar and anything else preceding it. Keep in mind that you're using position: absolute here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find away to use the "remaining" height of the screensize. Therefore I had to stick to absolute positioning and bottom: 0. To make the navbar visible the homescreen component is now offset to the background. It will take 100% of the height and width all the time but no more. As I think the main screen of this application will remain quite clean I think it should be alright that way.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have used the flexbox algorithm on the element enclosing the navbar and the homescreen component, but moving the homescreen component to the background should be just fine for now. We might address this in a future PR, however...

Comment thread frontend/src/components/mdx/homescreen/homescreen.module.scss Outdated
Comment thread frontend/src/components/mdx/homescreen/homescreen.module.scss Outdated
Comment thread frontend/src/components/mdx/info/info.module.scss Outdated
@LinkKF LinkKF requested a review from Quaffel March 29, 2021 12:48
Copy link
Copy Markdown
Owner

@Quaffel Quaffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good improvement, but please address the issues that are still open.
Would also be great if you could use a proper formatter in the SCSS files (mind the missing space character between the colons and the properties' actual values).

Comment thread frontend/src/components/mdx/image/image.tsx
Comment thread frontend/src/components/mdx/homescreen/homescreen.module.scss Outdated
left: 0;
bottom: 0;
right:0;
width:100vw;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have used the flexbox algorithm on the element enclosing the navbar and the homescreen component, but moving the homescreen component to the background should be just fine for now. We might address this in a future PR, however...

@Quaffel Quaffel mentioned this pull request Mar 30, 2021
@LinkKF LinkKF requested a review from Quaffel March 30, 2021 17:03
Comment thread frontend/src/components/navbar/navbar.module.scss Outdated
@LinkKF LinkKF requested a review from Quaffel March 30, 2021 18:54
Copy link
Copy Markdown
Owner

@Quaffel Quaffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really sorry for bothering you again, but I still want you to do two minor things:

  • Please restore the original package.json (The items are rearranged on npm installs)
  • Remove the redundant package-lock.json in the project's root.

I promise, you'll be good to go then 👼

@LinkKF LinkKF requested a review from Quaffel March 30, 2021 20:02
@LinkKF
Copy link
Copy Markdown
Collaborator Author

LinkKF commented Mar 30, 2021

Finally, WOOHOO

@LinkKF LinkKF merged commit 083a662 into development Mar 31, 2021
@LinkKF LinkKF deleted the new-components branch March 31, 2021 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants